Pull request overview

This PR refactors the index-parameter mapping layer to replace string-template JSON defaults and mapping tables with structured default builders plus explicit flat-key translation, while also centralizing RaBitQ split handling and adding a compatibility-report API to surface all JSON differences.

Changes:

• Replaces template-based default parameter JSON generation with structured builders and explicit per-key mapping for multiple index entry points (HGraph, Pyramid, IVF, BruteForce/WARP, SIMQ).
• Introduces CompatibilityReport / CollectCompatibilityIssues() to collect all JSON differences while preserving the existing boolean compatibility check.
• Centralizes RaBitQ split parsing/application and removes downstream “split-version” mutation; adds boundary validation for flat external keys (e.g., SINDI, SIMQ).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

    static const std::unordered_set<std::string> supported_keys = {
        SPARSE_TERM_ID_LIMIT,
        SPARSE_DOC_PRUNE_RATIO,
        USE_REORDER_KEY,
        USE_QUANTIZATION,
        SPARSE_WINDOW_SIZE,
        SPARSE_AVG_DOC_TERM_LENGTH,
        SPARSE_DESERIALIZE_WITHOUT_FOOTER,
        SPARSE_DESERIALIZE_WITHOUT_BUFFER,
        SPARSE_REMAP_TERM_IDS,
        SPARSE_RERANK_TYPE,
        SPARSE_DMQ_SHARED_CODEBOOK_THRESHOLD,
        SPARSE_IMMUTABLE,
    };

    static const std::unordered_set<std::string> keys = {BRUTE_FORCE_BASE_IO_TYPE,
                                                         BRUTE_FORCE_BASE_FILE_PATH,
                                                         "init_cluster_ratio",
                                                         "max_cluster_size",
                                                         "split_start_idx",
                                                         "random_seed",
                                                         "coarse_k",
                                                         "rerank_k"};

                        report.issues.push_back({child_path, "field is missing"});

                        report.issues.push_back({path + "." + key, "unexpected field"});

    REQUIRE(report.issues.size() == 3);
    REQUIRE(report.issues[0].path == "$.first");
    REQUIRE(report.issues[1].path == "$.nested.second");
    REQUIRE(report.issues[2].path == "$.extra");

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

    json[NO_BUILD_LEVELS].SetJson(JsonType::Parse("[]"));

#include <vector>

struct CompatibilityIssue {
    std::string path;
    std::string message;
};

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

        } else if (key == STORE_RAW_VECTOR) {
            inner_json[QUANTIZATION_PARAMS_KEY][HOLD_MOLDS].SetJson(field);
        } else if (key == USE_ATTRIBUTE_FILTER) {

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

void

        } else if (key == STORE_RAW_VECTOR) {
            inner_json[QUANTIZATION_PARAMS_KEY][HOLD_MOLDS].SetJson(field);

JsonType

JsonType

JsonType

JsonType

#include <shared_mutex>
#include <unordered_map>
#include <vector>

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Thoroughly reviewed the full diff (32 files, ~1200 additions, ~1500 deletions). The refactoring is well-executed and addresses all major concerns from earlier review rounds:

Verified fixes from previous Copilot comments:

• STORE_RAW_VECTOR: Both base and precise codes now correctly use ApplyHoldMoldsToQuantizer() for hold_molds propagation
• size_t → uint64_t: All new code uses uint64_t consistently
• SplitString trimming: Confirmed working, normalizes TQ chain whitespace
• Test ordering: Fixed with std::map

Architecture improvements:

• Template-based mapping replaced with structural CreateDefault() factories → no more string template mutation chains
• RaBitQSplitConfig struct + ParseRaBitQSplitConfig/ApplyRaBitQSplitConfig centralizes split configuration
• CompatibilityReport/CollectCompatibilityIssues() provides structured diff reporting alongside existing CheckCompatibility
• All mapping functions reject unknown flat keys with "invalid config param" errors
• SINDI/SINDIV2 now validate unknown flat keys at boundary

One remaining minor issue (already flagged by Copilot, not re-flagging):

• inner_index_parameter.cpp:105: error message says "mrle, rabitq" but SplitString normalizes whitespace so the comparison is against "mrle,rabitq". The error message could mislead users.

No new substantive issues found. The PR is in good shape.

Code Review Summary

Commit reviewed: a11cf1c

All issues flagged in the previous Copilot review have been addressed:

1. STORE_RAW_VECTOR — ApplyHoldMoldsToQuantizer is now correctly applied to both base_codes and precise_codes quantizers in bruteforce.cpp and hgraph_param_mapping.cpp.

2. size_t → uint64_t — Changed in parameter.h to match VSAG coding standards.

3. Test ordering dependency — Fixed by using std::map for order-independent assertions in fp32_quantizer_parameter_test.cpp.

4. TQ TYPE_KEY — Now properly set in TransformQuantizerParameter::ToJson() at transform_quantizer_parameter.cpp:112.

5. TQ whitespace — SplitString trims whitespace; verified by test.

Additional observations (no action needed)

• JSON pointer escaping (escape_json_pointer_token / append_json_pointer_token) is RFC 6901 compliant. The test correctly verifies "/a~1b" for key "a/b".
• RaBitQSplitConfig struct with ParseRaBitQSplitConfig (validates external params) + ApplyRaBitQSplitConfig (mutates inner JSON) is a clean separation of concerns. The shared key "base_quantization_type" works correctly for both HGraph and Pyramid since HGRAPH_BASE_QUANTIZATION_TYPE and PYRAMID_BASE_QUANTIZATION_TYPE have the same string value.
• build_default_flatten_param double-application of hold_molds (via CreateDefault + ApplyHoldMoldsToQuantizer) is intentional: CreateDefault handles FP32/INT8 directly, while ApplyHoldMoldsToQuantizer unwraps TQ chains to reach the bottom quantizer.
• SINDI/SINDIV2 now validate external params against a supported_keys set, rejecting unknown keys with a clear error message.
• FlattenDataCellParameter::CreateDefault correctly sets hold_molds on FP32/INT8 quantizer parameter objects, and ToJson serializes it.
• Naming convention — new helper functions use snake_case (build_default_*), consistent with the existing codebase style.
• Test coverage — regression tests added for split config, compatibility reporting, and hold_molds behavior.

No new substantive issues found. The refactoring is clean and well-tested.

This push addresses all previously raised review comments:

1. STORE_RAW_VECTOR path — Now correctly writes to base_codes.quantization_params and precise_codes.quantization_params via ApplyHoldMoldsToQuantizer, with a regression test added in bruteforce_parameter_test.cpp.

2. size_t → uint64_t — The CollectCompatibilityIssues loop in parameter.cpp now uses uint64_t for array traversal, consistent with project conventions.

3. Test ordering dependency — The CompatibilityReport test in fp32_quantizer_parameter_test.cpp now uses std::map for order-independent assertions, and also covers JSON pointer escaping (/a~1b).

4. Chain whitespace — SplitString already trims whitespace around tokens, so "mrle, rabitq" and "mrle,rabitq" are both handled correctly. A dedicated test in transform_quantizer_parameter_test.cpp verifies this.

No new issues found. The refactoring is clean, well-tested, and all prior concerns are resolved.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

        auto parameter = MultiVectorDataCellParameter::CreateDefault(IO_TYPE_VALUE_BLOCK_MEMORY_IO);

Review Summary

This is a large, well-executed refactoring that replaces template-based JSON parameter mapping (ConstParamMap + string templates) with explicit build_default_* functions and if-else chains across all index types. The PR also introduces CompatibilityReport/CompatibilityIssue for structured compatibility checking and RaBitQSplitConfig parsing/application.

What's been addressed from prior reviews

• STORE_RAW_VECTOR now correctly maps to base_codes.quantization_params.hold_molds and precise_codes.quantization_params.hold_molds via ApplyHoldMoldsToQuantizer
• parameter.h includes <string> for CompatibilityIssue/CompatibilityReport
• sindi.cpp includes <unordered_set>
• Test ordering issue fixed by using std::map for order-independent assertions
• FlattenDataCellParameter::CreateDefault now handles hold_molds for both FP32 and INT8
• SplitString trims whitespace, so chain parsing handles spaces correctly
• Unused build_default_quantization_param and stale includes removed

Items flagged in this review

1. Error message format (inner_index_parameter.cpp:105): The error message shows tq_chain="mrle, rabitq" (with space), but SplitString normalizes whitespace, so the no-space variant is also valid. Consider using the canonical normalized form in the error message.
2. Performance note (parameter.cpp:29): CollectCompatibilityIssues does a full JSON round-trip for comparison. Fine for a non-hot-path compatibility check, but worth noting.
3. Consistency note (hgraph_param_mapping.cpp:295): SINDI/SINDI v2 use a supported_keys whitelist for validation; the other index types use if-else chains with a catch-all throw. Both approaches are functionally correct, but consistency would improve maintainability.

Overall assessment

The refactoring is sound. The explicit mapping approach is more readable and maintainable than the template-based system it replaces. All previously flagged issues have been addressed. No correctness or security issues found.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.

[note] In src/parameter.cpp:92, common_size is derived from nlohmann::json::size() which returns size_t, but the loop iterates with uint64_t i. This can produce a signed/unsigned mismatch warning on platforms where size_t is narrower than uint64_t (e.g., 32-bit) or trigger -Wsign-compare warnings.

Consider using size_t for the loop index to match the container size type, or casting common_size explicitly to uint64_t if the project convention requires uint64_t everywhere:

for (size_t i = 0; i < common_size; ++i) {

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.

Review Summary

This PR replaces template-based JSON parameter mapping with structural builders and explicit flat-field translation across all index types. The refactoring is thorough and well-structured, with several improvements over the previous approach.

Issues Fixed from Previous Reviews

• STORE_RAW_VECTOR now correctly applies ApplyHoldMoldsToQuantizer to both base and precise codes in BruteForce, HGraph, and Pyramid
• size_t replaced with uint64_t in CollectCompatibilityIssues
• Test ordering dependency fixed with std::map
• SplitString whitespace trimming already handled

New Additions

• CompatibilityReport / CompatibilityIssue with JSON pointer path escaping (~0 for ~, ~1 for /) provides structured compatibility checking
• RaBitQSplitConfig parsing and ApplyRaBitQSplitConfig centralize split quantizer configuration
• ApplyHoldMoldsToQuantizer propagates hold_molds flag through TQ chains to supported quantizers (fp32, int8)
• ValidateMRLEDim and RequiresRawVectorFor* helpers consolidate validation logic
• CreateDefault factory methods on parameter classes improve testability and code reuse
• Whitelist-based validation for SINDI/SINDIV2 parameters provides clear error messages

One Minor Note

• build_default_simq_param defaults to IO_TYPE_VALUE_ASYNC_IO while all other builders default to IO_TYPE_VALUE_BLOCK_MEMORY_IO. This is consistent with the old template, but worth confirming the asymmetry is intentional.

Overall

The refactoring eliminates the fragile string-template mutation chain and replaces it with structured, type-safe builders. The explicit if-else chains are verbose but clear and maintainable. No blocking issues found.